Add additional jar names to classpathFromResources for types referenced from types used#160
Merged
Add additional jar names to classpathFromResources for types referenced from types used#160
classpathFromResources for types referenced from types used#160Conversation
diff --git c/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java i/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java index a9a22d2..58f1dea 100644 --- c/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java +++ i/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java @@ -68,12 +68,9 @@ public class TemplateCode { jarNames.addAll(ClasspathJarNameDetector.classpathFor(parameter, ImportDetector.imports(parameter))); } if (!jarNames.isEmpty()) { - // It might be preferable to enumerate exactly the needed dependencies rather than the full classpath - // But this is expedient - // See #86 - // String classpath = jarNames.stream().map(jarName -> '"' + jarName + '"').sorted().collect(joining(", ")); - // builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpath(").append(classpath).append("))"); - builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath()))"); + builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "); + builder.append(jarNames.stream().map(jarName -> '"' + jarName + '"').collect(joining(", "))); + builder.append("))\n "); } return builder.toString(); } catch (IOException e) {
diff --git c/src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java i/src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java index ab59a8a..8a4b126 100644 --- c/src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java +++ i/src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java @@ -19,6 +19,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCFieldAccess; import com.sun.tools.javac.tree.TreeScanner; +import org.jspecify.annotations.Nullable; import javax.tools.JavaFileObject; import java.util.LinkedHashSet; @@ -38,7 +39,7 @@ public class ClasspathJarNameDetector { public static Set<String> classpathFor(JCTree input, List<Symbol> imports) { Set<String> jarNames = new LinkedHashSet<String>() { @OverRide - public boolean add(String s) { + public boolean add(@nullable String s) { return s != null && super.add(s); } }; @@ -64,7 +65,7 @@ public class ClasspathJarNameDetector { } - private static String jarNameFor(Symbol anImport) { + private static @nullable String jarNameFor(Symbol anImport) { Symbol.ClassSymbol enclClass = anImport instanceof Symbol.ClassSymbol ? (Symbol.ClassSymbol) anImport : anImport.enclClass(); while (enclClass.enclClass() != null && enclClass.enclClass() != enclClass) { enclClass = enclClass.enclClass(); diff --git c/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java i/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java index 58f1dea..337a65a 100644 --- c/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java +++ i/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java @@ -68,9 +68,11 @@ public class TemplateCode { jarNames.addAll(ClasspathJarNameDetector.classpathFor(parameter, ImportDetector.imports(parameter))); } if (!jarNames.isEmpty()) { - builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "); - builder.append(jarNames.stream().map(jarName -> '"' + jarName + '"').collect(joining(", "))); - builder.append("))\n "); + String joinedJarNames = jarNames.stream().collect(joining(", ", "\"", "\"")); + builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, ") + .append(joinedJarNames) + .append("))\n "); + } return builder.toString(); } catch (IOException e) {
…untimeclasspath-in-remaster-templates
timtebeek
commented
Aug 15, 2025
src/test/resources/refaster/ClasspathFromResourcesTransitiveRecipe.java
Outdated
Show resolved
Hide resolved
Base automatically changed from
1136-use-classpathfromresources-instead-of-runtimeclasspath-in-remaster-templates
to
main
August 15, 2025 13:47
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
I think what is required is to include the jars of all types directly referenced as either supertypes or in method signatures of the types referenced by the template. I don't think the full transitive closure is required. |
src/test/java/org/openrewrite/java/template/internal/ClasspathJarNameDetectorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/template/internal/ClasspathJarNameDetectorTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/template/internal/ClasspathJarNameDetector.java
Outdated
Show resolved
Hide resolved
3 tasks
timtebeek
commented
Aug 18, 2025
Comment on lines
+56
to
+57
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | ||
| .replaceAll("(-\\d+).*?$", "$1"); |
Member
Author
There was a problem hiding this comment.
it's subtle, but since we match on prefixes on the other side we need a way to tell these apart. I've deliberately not pinned to more specific versions, as we use latest.release often, and I don't see overlap yet beyond major versions even for rewrite-spring.
Alternatively we could on the other end match based on "jarname" + '-' to tell these apart without pinning to major versions.
Contributor
There was a problem hiding this comment.
I think this is good enough! There's always the option to handle this differently in another PR 😉.
Contributor
There was a problem hiding this comment.
Indeed looks good as a first step. Possibly we can restrict it a bit more (untested), but I am also fine with leaving it as-is:
Suggested change
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | |
| .replaceAll("(-\\d+).*?$", "$1"); | |
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | |
| .replaceFirst("(-\\d+)", "$1"); |
Member
Author
There was a problem hiding this comment.
Thanks! With that original suggestion the tests fail with the following:
value of : classpathForSource(...)
missing (2) : junit-jupiter-api-5, opentest4j-1
unexpected (2): junit-jupiter-api-5.13.3, opentest4j-1.3.0
Note that my intention was to only retain the major version, if present, and discard the rest, hence why I've included the non greedy match with end of string: .*?$. I did apply your suggestion to use replaceFirst, even if the inclusion of $ would result in the same.
knutwannheden
approved these changes
Aug 19, 2025
Comment on lines
+56
to
+57
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | ||
| .replaceAll("(-\\d+).*?$", "$1"); |
Contributor
There was a problem hiding this comment.
Indeed looks good as a first step. Possibly we can restrict it a bit more (untested), but I am also fine with leaving it as-is:
Suggested change
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | |
| .replaceAll("(-\\d+).*?$", "$1"); | |
| // Retain major version number, to avoid `log4j` conflict between `log4j-1` and `log4j2-1` | |
| .replaceFirst("(-\\d+)", "$1"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Right now we add the transitive jar names only when referred to from types we're using.
I've wondered whether we should unconditionally add say "opentest4j" whenever any method from JUnit is used, but figured that might quickly result in a lot of classpathFromResources entries that would then need to be added, so I've opted to for now only add those when referred by the types we use in the templates, as can be seen for the difference between the tests we now have for ClasspathJarNameDetector.